Skip to content
This repository was archived by the owner on Sep 6, 2021. It is now read-only.

Fix shapes update#7424

Merged
RaymondLim merged 7 commits intoadobe:masterfrom
oslego:fix-shapes-update
Apr 7, 2014
Merged

Fix shapes update#7424
RaymondLim merged 7 commits intoadobe:masterfrom
oslego:fix-shapes-update

Conversation

@oslego
Copy link
Copy Markdown
Contributor

@oslego oslego commented Apr 5, 2014

Attempts to recover from errors by reconnecting the editor. If that's not possible, turns off everything so the user can trigger it again manually.

These are invalid shape declarations and the editor will turn off:

  • circle(px at 50% 50%)
  • circle(50% 50%)
  • circle(50%, 50%)

This is a valid shape declaration: circle(50% at) but it crashes the browser due to an implementation bug in the browser. The issue has been fixed, but it will take time for the fix to trickle down into Chrome Stable. That notation will crash the browser before the shapes editor has a chance to load and attempt to catch it.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some typos. Should be otherwise the _reconnect() scenario misses the cache and fails

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One typo: otherwhise

@RaymondLim
Copy link
Copy Markdown
Contributor

@oslego Done with review. You also need to remove a duplicate unit test on line 470-479 since it is already on line 460-469.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still one typo: otherwhise

@RaymondLim
Copy link
Copy Markdown
Contributor

@oslego Looks good. Before I merge it, do you want to remove _getRangeForCSSValueAt and the related unit tests?

@oslego
Copy link
Copy Markdown
Contributor Author

oslego commented Apr 7, 2014

Will do it in a few hours. Need to step out for a while. Either the merge can wait or I can do another PR later with the removal.

@RaymondLim
Copy link
Copy Markdown
Contributor

Merging now since we need to investigate a couple of issues with the latest code.

RaymondLim added a commit that referenced this pull request Apr 7, 2014
@RaymondLim RaymondLim merged commit 8f3fc7b into adobe:master Apr 7, 2014
@oslego oslego deleted the fix-shapes-update branch April 9, 2014 14:43
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@oslego We should also return if (!data). Otherwise, we need to check data again before accessing it on line 153 data.forceUpdate.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants